Review requested:

• @nodejs/http2
• @nodejs/net

Can you use "-s" (for Signed-off-by) in first commit command, after run this command
example: git commit --amend --no-edit -s "http2: avoid uaf while receiving and sending rst_stream"

after:
CLANG_FORMAT_START=$(git merge-base HEAD main) make format-cpp for cpp lint errors

Is everything correct now?

Can you add a test case or an script that we could reproduce it?

The scripts are in the issue #64113
The error is reproduced when using the ASAN sanitizer.

If you mean replace the code with this

  if (session_->is_receiving()) {
    if (is_stream_cancel(code) || available_outbound_length_ > 0) {
      session_->AddPendingRstStream(id_);
      return;
    }
    FlushRstStream();
    return;
  }

then it fixes uaf, but I get these failed tests:
test-http2-compat-errors
test-http2-destroy-after-write
test-http2-server-errors

Or if I misunderstood you, please write your version.

Ah, interesting, yes we'll have to investigate that @Eusgor.

I took a bit more of a look, I think this is part of a bigger more fundamental issue - the nghttp2 docs say:

Do not call nghttp2_session_send(), nghttp2_session_mem_send2(), nghttp2_session_recv() or nghttp2_session_mem_recv2() from the nghttp2 callback functions directly or indirectly. It will lead to the crash.

And in this method, we call SendPendingData while receiving.

The tests are reasonable (e.g. destroy after write - it should flush the written data before the stream is destroyed) but this method we're changing isn't - it should never call SendPendingData with is_receiving() === true.

I had Claude do a quick scan, there's at least three places we ignore this nghttp2 requirement:

• Http2Stream::SubmitRstStream() - fixed by this PR, but only for the case where no writes are pending.
• Http2Session::OnDataChunkReceived - can call SendPendingData if buffer is too large.
• Http2Session::Close() - calls SendPendingData directly

All of those are issues (good find!) and the new is_receiving() flag you've added is the right way to fix all of them, but we should take a more general approach instead of fixing this small part of one case.

Can you take a look? Really I think we need to always defer anything anything that could call one of those methods while receiving - that means something like early-return in SendPendingData() if receiving, deferring resets if there is pending data, and deferred destroy/close to fix the test failures around this logic. Take a look, let me know if you need any pointers and I can come up with a more detailed outline if that'd help.